-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better onboarding #161
base: main
Are you sure you want to change the base?
Better onboarding #161
Conversation
gitterHostDefault = "127.0.0.1" | ||
gitterPortDefault = "1234" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated below to use environment variables (so that this code can run in a docker aside the gitter runner), and these defaults were kept for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the contribution @tyron and my apologies for the delayed review! Mostly looks good, I think we can drop the Dockerfile
changes though 👍🏻
# Copy internal libraries. | ||
COPY . . | ||
# Copy and cache dependencies | ||
COPY ../go.mod . | ||
COPY ../go.sum . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Dockerfile
typically gets invoked from root via the Makefile
so I think we can keep this as is (the proposed change will not work with make build-doc
or make build-gitter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make build-doc
and make build-gitter
both work for me. Did you have any problems?
Up to you, I just feel these changes make it easier to iterate during development.
Analyzing execution of make build-doc
...
Based on the main
branch, the 1st execution took me 78s. Another execution on top of that without any changes was quick, 1.8s. Then I just added a comment to deploy/doc.yaml
(echo "\n# test" >> deploy/doc.yaml
), increased timing to 56s.
Now with my proposed changes, the 2 first executions were very similar. But the 3rd execution, which has an updated file, took just 13s (vs 56s).
Again, I'm not working often with the repo to say if that's important. But when I had to build the image several times, that saved me quite some time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for reference, running these before go mod download
is suggested in golang's Dockerhub under "Start a Go instance in your app"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyron yep I am on board with copying just the go.mod
/ go.sum
files in for go mod download
to ensure that we can cache and re-use the intermediate image 👍🏻 The Makefile
is in the root of the directory though, as is the build context, so your relative paths to ../go.mod
and ../go.sum
should just be go.mod
and go.sum
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW we can also mount the go build cache as well if you want to speed it up even more :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so I tested a bit more because I was curious on how my setup was working.
I believe the notation in the Dockerfile is not based on your current pwd, yet relative to Dockerfile itself. From what I can test, even if your build context is the root folder and the go.mod is in the root folder, it should still be written as ../go.mod
if Dockerfile is in a subfolder.
So since go.{mod,sum}
are always on the parent folder of deploy
, writing as ../go.{mod,sum}
always returns the proper location. Does it makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I don't understand anything. Both COPY ../go.mod .
and COPY go.mod .
seem to work.
@@ -3,15 +3,19 @@ | |||
# https://hub.docker.com/_/golang | |||
FROM golang:1.13 as builder | |||
|
|||
WORKDIR app/ | |||
WORKDIR /go/app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is following best-practices of using an absolute path instead of relative.
go mod download
)